Fix extra incorrectly shown on installation error - #13659
Conversation
|
I've not spent any time looking at the code, but I would be pretty nervous about removing the marker information until right before the error message. Even if that marker information isn't used now someone might want to use it later and be surprised to find all marker information stripped, and there's no comment as to why that happened. |
Thanks for the review, @notatallshaw! |
notatallshaw
left a comment
notatallshaw
left a comment
There was a problem hiding this comment.
Update the test test_install_special_extra in tests/functional/test_install_extras.py to include something like this at the end:
assert "No matching distribution found for missing_pkg" in result.stderr, str(result)
assert "extra ==" not in result.stderr, str(result)Then I think you're going to find two situations you're not handeling right now:
- When the requirement has a parent, you need to move your string manipulation to the top of
_report_single_requirement_conflictso it also gets applied to thereq_dispvariable for logging - The logging and raising of raise DistributionNotFound in
PackagingFinder.find_requirementinsrc/pip/_internal/index/package_finder.py
|
I'm realizing I wrote the above comment in a bit of a rush and I've not verified the output is actually misleading in that test. So feel free to correct me if I made a mistake, I'll take a other look in the next few days when I have time to review again. |
|
I'm going to move this to 26.1 milestone, I'm generally happy with this approach, but it needs tests, either as new or modified existing ones, and I don't see a rush to get it out (this error has been confusing for a long time). |
|
Hi @notatallshaw, thanks for the review. Sorry for the delay. I’ve updated the code and tests based on your feedback. Would appreciate another look when you have a moment. |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Not a pip developer, but hopefully some helpful review comments...
Also this needs more tests I think, right now it seems that only one of the two modified paths is tested?
|
Hi @danielhollas, thanks a lot for the helpful review! I've addressed your comments, but I’m not entirely clear on your note about the assertion test. Could you clarify what you meant? |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
@sepehr-rs thanks. You can disregard my test assertion comment, but I think there's still an issue with how you override req_disp variable.
|
I took a quick look at this danielhollas's comments all look correct to me. |
|
Hi, sorry for the delay. Things have been hectic over here in Iran and I only recently got access to GitHub again. I’ve committed the changes proposed by @danielhollas. Please let me know what you think! |
|
Sorry, I think this PR still needs some more review and work, but I'm very unlikely to have time to review before 26.1. |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Things have been hectic over here in Iran and I only recently got access to GitHub again.
Oof, what an understatement, I am very sorry. :-(
Thank you for the changes, I think that there is at least one thing that needs fixing, otherwise looks reasonable to me, but I am not a pip maintainer, just trying to help with a review :-)
Co-authored-by: Daniel Hollas <danekhollas@gmail.com>
Co-authored-by: Daniel Hollas <danekhollas@gmail.com>
|
Gentle ping. I believe I've addressed all review comments and follow-up feedback. I think this may be ready for another review. I'd appreciate any feedback when someone has time. |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Looks good to me, but not a pip maintainer. :-)
|
@notatallshaw I'll leave the review to you. I'm not a fan of using regex substitution, but on a quick glance, it doesn't seem like the packaging marker API allows us to filter out extra markers easily... |
|
I'm deferring this to pip 26.3. |
4 participants
Fixes #13618
This resolves the issue, though I’m not certain it’s the ideal place for the fix. If needed, we can move the solution further down in the stack.